Skip to content

fips: keep PRIVATE_KEY_UNLOCK/op/LOCK inside a single cgo call - #59

Merged
sebastian-carpenter merged 1 commit into
wolfSSL:masterfrom
lealem47:lock_unlock
Sep 3, 2026
Merged

fips: keep PRIVATE_KEY_UNLOCK/op/LOCK inside a single cgo call#59
sebastian-carpenter merged 1 commit into
wolfSSL:masterfrom
lealem47:lock_unlock

Conversation

@lealem47

@lealem47 lealem47 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

A goroutine is pinned to its OS thread only for the duration of a single
cgo call. Calling PRIVATE_KEY_UNLOCK, then the wolfCrypt operation, then
PRIVATE_KEY_LOCK as three separate cgo calls therefore leaves a window in
which the scheduler can move the goroutine to another thread. The
operation then runs on a thread whose counter was never incremented and
fails with FIPS_PRIVATE_KEY_LOCKED_E (-287), while the thread that took
the unlock is left permanently unlocked.

To address this, this PR moves the unlock/crypto call/lock into one static cgo helper function

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The current diff introduces build-breaking issues in the C preambles (undefined WC_SPKRE_F and C helpers calling PRIVATE_KEY_UNLOCK/LOCK which are not C symbols).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a FIPS-threading correctness issue by ensuring the “private-key gate” (unlock → wolfCrypt op → lock) happens within a single cgo call, preventing goroutine rescheduling onto a different OS thread mid-sequence.

Changes:

  • Add per-operation static C helpers that wrap unlock/op/lock inside one cgo call for HKDF, PBKDF2, and selected ECC operations.
  • Expand and clarify FIPS private-key gate documentation and threading requirements in fips.go.
File summaries
File Description
hmac.go Adds a static C HKDF wrapper intended to keep unlock/op/lock within one cgo call.
aes.go Adds a static C PBKDF2 wrapper intended to keep unlock/op/lock within one cgo call.
ecc.go Adds static C wrappers for ECC private export and shared secret to keep unlock/op/lock within one cgo call.
fips.go Documents the FIPS private-key gate/threading behavior; adjusts the C preamble for private-key lock/unlock helpers.
Review details

Suppressed comments (3)

fips.go:38

  • The WC_SPKRE_F macro is referenced by WC_PRIVATE_KEY_LOCK/UNLOCK but its definition was removed, leaving WC_SPKRE_F undefined in the C preamble (build will fail). Reintroduce the macro (or call wolfCrypt_SetPrivateKeyReadEnable_fips directly).
// int WC_PRIVATE_KEY_LOCK(void) {
//      return WC_SPKRE_F(0,WC_KEYTYPE_ALL);
// }
// int WC_PRIVATE_KEY_UNLOCK(void) {
//      return WC_SPKRE_F(1,WC_KEYTYPE_ALL);

ecc.go:148

  • This C helper calls PRIVATE_KEY_UNLOCK/LOCK, which are Go functions and not available to C. Switch to WC_PRIVATE_KEY_UNLOCK/WC_PRIVATE_KEY_LOCK and declare them to avoid implicit-declaration errors on clang.
//     PRIVATE_KEY_UNLOCK();
//     ret = wc_ecc_export_x963_ex(key, out, outLen, compressed);
//     PRIVATE_KEY_LOCK();

ecc.go:158

  • This C helper calls PRIVATE_KEY_UNLOCK/LOCK, which are Go functions and not available to C. Use WC_PRIVATE_KEY_UNLOCK/WC_PRIVATE_KEY_LOCK (and declare them) so the helper compiles/links correctly.
//     PRIVATE_KEY_UNLOCK();
//     ret = wc_ecc_shared_secret(private_key, public_key, out, outLen);
//     PRIVATE_KEY_LOCK();
  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread aes.go
Comment thread ecc.go
Comment thread hmac.go
Comment thread fips.go
@sebastian-carpenter
sebastian-carpenter merged commit e9060c6 into wolfSSL:master Sep 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants